Fix building the sanitizer instrumentation and hooking it into the hosts#116166
Conversation
github-actions
Bot
added
the
needs-area-label
dotnet-policy-service
Bot
added
the
community-contribution
jkotas
removed
the
needs-area-label
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Commenter does not have sufficient privileges for PR 116166 in repo dotnet/runtime |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple issues preventing AddressSanitizer (ASan) instrumentation from building and functioning correctly on Windows with MSVC. The changes ensure proper symbol export, correct calling conventions, and appropriate linker flag handling for sanitizer-enabled builds.
Key changes:
- Changed ASan callback calling convention from
__stdcallto__cdeclfor consistency and correctness - Simplified MSVC sanitizer callback macros by removing conditional dllexport logic
- Added .def file to properly export ASan symbols on Windows
- Disabled
/DEPENDENTLOADFLAG:0x800when sanitizers are enabled to avoid DLL loading issues
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/nativeaot/SmokeTests/SharedLibrary/SharedLibrary.cpp | Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention |
| src/tests/nativeaot/CustomMainWithStubExe/CustomMainWithStubExeNative.cpp | Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention |
| src/tests/nativeaot/CustomMain/CustomMainNative.cpp | Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention |
| src/native/minipal/utils.h | Simplified MSVC sanitizer macro definitions by removing conditional dllexport logic |
| src/native/minipal/sansupport.c | Added extern "C" linkage guards for C++ compatibility |
| src/native/minipal/sanitizer_exports.def | New file defining exports for ASan callback functions on Windows |
| src/native/minipal/CMakeLists.txt | Added .def file dependency and linker options for sanitizer support; disabled default build; typo in comment |
| src/native/corehost/CMakeLists.txt | Added minipal subdirectory to make sanitizer support available to corehost |
| src/coreclr/vm/interpexec.cpp | Reordered standard library includes before internal headers; removed trailing whitespace |
| src/coreclr/vm/common.h | Added GCSafeMemCpy to std namespace to fix macro redefinition issues with std::memcpy |
| src/coreclr/utilcode/ex.cpp | Simplified delete operator by removing redundant DACCESS_COMPILE conditional |
| src/coreclr/nativeaot/CMakeLists.txt | Added ASAN runtime DLL installation for NativeAOT |
| eng/native/configurecompiler.cmake | Extended condition to also skip DEPENDENTLOADFLAG when sanitizers are enabled |
Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
agocke
left a comment
agocke
left a comment
There was a problem hiding this comment.
LGTM aside from some questions about the GCSafeMemCpy usage
|
@39otsu, please check CI test failures. |
JulieLeeMSFT
added
the
needs-author-action
|
@39otsu Could you please resolve the merge conflicts?
The CI run is old. The results are no longer available. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "9bdee542462db9e5369d5067e429d00bfff8c7a0",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f7dfaf6882e030191b6a70e6441c4141887d8d3b",
"last_reviewed_commit": "00991677afe5d4b608fb3a3cc391b042608dabdf",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "c404ba174e46ce0ceb07bb2e7effb6030822bf5a",
"last_recorded_worker_run_id": "29676142410",
"review_attempt_commit": "9bdee542462db9e5369d5067e429d00bfff8c7a0",
"review_attempt_base_ref": "main",
"review_attempt_count": 1,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "00991677afe5d4b608fb3a3cc391b042608dabdf",
"review_id": 4730525531
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: This PR is a carry-over of #112378 and fixes several problems that prevented the AddressSanitizer instrumentation from building and working correctly with MSVC. It ensures the correct ASan callback symbols are exported when building the host/runtime as a DLL, corrects the calling convention and linkage of the public __asan_* callbacks, and prevents /DEPENDENTLOADFLAG:0x800 from forcing the ASan runtime DLL to be resolved only from System32. It also removes the debug-only GCSafeMemCpy memcpy override, which shadowed the C runtime memcpy and interfered with sanitizer instrumentation.
Approach: The changes are cohesive and mostly build-plumbing:
eng/native/configurecompiler.cmake: also skip/DEPENDENTLOADFLAG:0x800when sanitizers are enabled, matching the existing PGO carve-out.src/native/minipal/*: addsanitizer_exports.defand wire it in via/DEF:for MSVC so__asan_default_options/__asan_on_errorare exported; wrap the callbacks inextern "C"; add aset_source_files_properties(... OBJECT_DEPENDS ...)dependency so the object rebuilds when the def changes; simplifySANITIZER_CALLBACK_CALLCONV/SANITIZER_INTERFACE_CALLCONVon MSVC to plain__cdecl(dropping thedllexport/dllimportpath now that exports are handled via the .def file).src/native/corehost/CMakeLists.txt: add theminipalsubdirectory so the host can link the sanitizer support target.src/coreclr/nativeaot/CMakeLists.txt: install the dynamic ASan runtime alongside the NativeAOT output, consistent with the existing corerun/corehost install logic.src/coreclr/vm/common.h+object.cpp+inc/formattype.h: remove the_DEBUG-onlyGCSafeMemCpymemcpyredefinition (endorsed by@jkotasin review as low-value and a source of trouble with standard-header overrides).src/coreclr/utilcode/ex.cpp:Exception::Deletenow unconditionally usesdeleteinstead of the non-DAC::delete, which is a natural follow-on to removing the global operator-shadowing.- Test host files (
CustomMain*,SharedLibrary.cpp): change__asan_default_optionsfrom__stdcallto__cdecl, addressing@jkoritzinsky's note that the callback must becdeclon all platforms; the non-Windows#define __cdeclkeeps these compiling on Unix. src/coreclr/vm/interpexec.cpp: only whitespace/include-reordering cleanup, no behavioral change.
Summary: This is a focused, low-risk build/infrastructure fix that is confined to sanitizer-enabled and debug configurations plus test hosts; the retail runtime code paths are unaffected. The reasoning is sound, the changes are consistent with existing ASan install/export patterns in the repo, and prior reviewer feedback (calling convention, comment wording, GCSafeMemCpy removal) has been addressed. The PR is already approved by a runtime maintainer. The only issues I see are cosmetic: three files (sanitizer_exports.def, sansupport.c, and nativeaot/CMakeLists.txt) are missing a trailing newline (flagged inline). Verdict: LGTM.
Detailed Findings
No functional issues found. Two minor, non-blocking nits are noted inline:
- Missing trailing newline in
src/coreclr/nativeaot/CMakeLists.txt,src/native/minipal/sansupport.c, andsrc/native/minipal/sanitizer_exports.def.
Observations (non-actionable): The SANITIZER_INTERFACE_CALLCONV/SANITIZER_SHARED_RUNTIME machinery in utils.h is now only meaningful on the non-MSVC branch; if the shared-runtime import path is no longer exercised on Windows this is fine, but worth keeping in mind for future cleanup. The ASan symbol exports rely on the .def file being applied via the object target's INTERFACE link options to whatever final binary links minipal_sanitizer_support; this depends on that target being linked into each host/runtime binary that needs the callbacks, which matches the added add_subdirectory in corehost.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 107.2 AIC · ⌖ 11.1 AIC · ⊞ 10K
dotnet-policy-service
Bot
removed
the
needs-author-action
There was a problem hiding this comment.
Holistic Review
Motivation: This PR (carry-over of #112378) fixes several problems preventing the AddressSanitizer instrumentation from building and working correctly with MSVC: exporting the correct __asan_* callback symbols when building as a DLL, correcting their calling convention and linkage, preventing /DEPENDENTLOADFLAG:0x800 from forcing the ASan runtime DLL to resolve only from System32, and removing the debug-only GCSafeMemCpy memcpy override that shadowed the CRT memcpy and interfered with instrumentation.
Approach: The changes remain cohesive build-plumbing: skip /DEPENDENTLOADFLAG:0x800 under sanitizers; add sanitizer_exports.def wired via /DEF: and extern "C" callbacks in src/native/minipal; simplify the MSVC callback calling convention to plain __cdecl now that exports go through the .def file; install the dynamic ASan runtime alongside NativeAOT output; remove the _DEBUG-only GCSafeMemCpy redefinition (common.h/object.cpp/formattype.h) and the corresponding ::delete in ex.cpp; and switch test-host __asan_default_options from __stdcall to __cdecl.
Summary: The current PR patch is materially unchanged from the previously reviewed head. All 17 commits are identical in the range-diff; the only differences from the prior scope are rebase artifacts against a newer merge base — the src/native/corehost/CMakeLists.txt add_subdirectory(minipal) hunk and the interpexec.cpp whitespace/include cleanup have been absorbed into main and are no longer part of the PR's incremental patch. These are not new authored changes and introduce no new findings. This remains a focused, low-risk build/infrastructure fix confined to sanitizer-enabled and debug configurations plus test hosts, leaving retail runtime paths unaffected, and it is already approved by a runtime maintainer. Verdict: LGTM.
Assessment History
- review 4730525531 reviewed commit
00991677afe5d4b608fb3a3cc391b042608dabdfwith verdict LGTM. Current verdict: LGTM. Assessment unchanged — motivation, approach, and risk are the same; the incremental delta since that review is only a rebase onto a newer base that upstreamed two previously PR-carried hunks, with no change to the PR's authored patches.
Detailed Findings
No new findings. The PR patch has not changed in substance since the prior review; the prior review's two non-blocking trailing-newline nits (in sanitizer_exports.def, sansupport.c, and nativeaot/CMakeLists.txt) still stand and are unchanged by this rebase.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 59.2 AIC · ⌖ 10.6 AIC · ⊞ 10K
Carry-over from @jkoritzinsky's PR: #112378
This PR addresses multiple issues that were preventing the ASan instrumentation from building and functioning as intended.
Verified successful build and test run with VS 17.14 p7, MSVC 14.44.35207